Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use X-Forwared-Host for resolving host #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frankbille
Copy link

X-Forwarded-For is normally used for getting the original client IP, whereas X-Forwarded-Host is normally used to get the original host requested by the client.

X-Forwarded-For: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
X-Forwarded-Host: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host

X-Forwarded-For is normally used for getting the original client IP, whereas X-Forwarded-Host is normally used to get the original host requested by the client.

X-Forwarded-For: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
X-Forwarded-Host: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-Host
@tboerger
Copy link
Contributor

Even if that is correct this can be a breaking change for various installations.

@tboerger
Copy link
Contributor

And beside that, this the "for" header works totally fine with reverse proxies like traefik, caddy or haproxy.

@frankbille
Copy link
Author

frankbille commented Jul 22, 2017

It was actually because of traefik I found this. The latest traefik version sets X-Forwarded-For to an IP address, and X-Forwarded-Host to the original requested hostname.

I understand the point about backward compatibility. Maybe if both were supported?

@tboerger
Copy link
Contributor

Maybe it should be better to check for host and as a fallback check the for header.

The traefik change have been integrated within the last release?

@frankbille
Copy link
Author

I am fairly new to Traefik, but I think they have been using Oxy for a long time (It is the library handling the proxying). And Oxy has been using X-Forwarded-For for client ip for at least 3 years. So this is how traefik has been handling this always.

I can't think of a good way to support both headers for the same thing other than configuration. If we handle both, then it has to be either:

  1. X-Forwarded-Host
  2. X-Forwarded-For

or

  1. X-Forwarded-For
  2. X-Forwarded-Host

But since both fields will most likely be filled out in most cases, you would always take the first one in both cases.

@tboerger
Copy link
Contributor

I'm using traefik already quite long and it always worked fine for me in combination with this middleware

@frankbille
Copy link
Author

That is quite odd. Have you checked that the X-Forwarded-For header is set? And it is an actual hostname and not an IP?
I'm also reading up on this now again, and I can't find any that uses that header for anything but client ip.

@tboerger
Copy link
Contributor

There is an pass host Header Option I'm using with traefik

@frankbille
Copy link
Author

frankbille commented Jul 23, 2017

But that passes the "Host" header, and according to the resolveHost algorithm it will never get to that if the X-Forwarded-For header is set.

The algorithm is:

  1. X-Forwarded-For header (I think this is incorrect, as it should be X-Forwarded-Host)
  2. X-Host header (a fine use of older header)
  3. request.Host (This uses host from request url and if not present uses the "Host" header)
  4. Host from request url (repetition from third step and thus superfluous)
  5. Default host from config

So in your case with traefik you have to somehow suppress the passing of X-Forwarded-For or do something else instead. Have you tried to debug what headers you get from the traefik request?

@tboerger
Copy link
Contributor

I will prepare a docker-compose scenario to see the exact behavior

@jdolitsky
Copy link

Can we instead add a field to the Config struct, such as ForwardingHeader, that is used for this? If not provided, just defaults to "X-Forwarded-For" so as not to break anyones setup?

location.go:

...
	// ForwardingHeader is the expected header for identifying the original host
	// requested by the client in the Host HTTP request header
	ForwardingHeader string
}

config.go:

func newLocation(config Config) *location {
	forwardingHeader := config.ForwardingHeader
	if forwardingHeader == "" {
		forwardingHeader = "X-Forwarded-For"
	}
	return &location{
		scheme: config.Scheme,
		host:   config.Host,
		base:   config.Base,
		headers: headers{
			scheme: "X-Forwarded-Proto",
			host:   forwardingHeader,
		},
	}
}

cc @hho

@kinnou02
Copy link

What have to be done to make this PR go forward?
The solution proposed by @jdolitsky seems to solve everyone problems.
I can write a bit of documentation if it is the last thing needed.

kinnou02 added a commit to kinnou02/gormungandr that referenced this pull request Apr 25, 2018
Use an updated version of gin-contrib/location to get the correct host
PR already opened here: gin-contrib/location#13

fix #49
kinnou02 added a commit to kinnou02/gormungandr that referenced this pull request Apr 30, 2018
Use an updated version of gin-contrib/location to get the correct host
PR already opened here: gin-contrib/location#13

fix #49
@eric
Copy link

eric commented Nov 15, 2020

I just ran into this with traefik as well and was very confused by what I was seeing.

@ihipop
Copy link

ihipop commented Dec 23, 2020

WHY THIS IS NOT MERGED?
we can use incompatible version change via go mod

@TJM
Copy link

TJM commented Feb 24, 2021

I agree, bump the major version and get this released, but for anyone else who is using this module and needs a quick workaround:

	// location - detects the scheme (http/https) and hostname of the server via the http.Request
	locConfig := location.DefaultConfig()
	locConfig.Headers.Host = "X-Forwarded-Host" // Workaround BUG in location middleware
	router.Use(location.New(locConfig))

@neumantm
Copy link

neumantm commented Sep 9, 2022

Agree. Just ran into the same thing with kubernetes ingress-nginx.

In my opinion using X-Forwaeded-For is a plain bug.
I can see no scenario where this is/was really what a developer wants (except if the reverse proxy has also a matching bug, but then that should also be addressed there.)
But if you want to be really safe to not break existing things: Bump the major version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants